[ResponseOps][Rules] Remove unintended internal Find routes API with public access #193757

Zacqary · 2024-09-23T16:47:33Z

Summary

Fixes #192957

Removes the internal/_find route from public access by moving the hard-coded options into the route builder functions.

…public access

elasticmachine · 2024-09-23T16:47:36Z

Pinging @elastic/response-ops (Team:ResponseOps)

obltmachine · 2024-09-23T16:47:47Z

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

/oblt-deploy : Deploy a Kibana instance using the Observability test environments.
run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

Zacqary · 2024-09-23T16:48:08Z

@elasticmachine merge upstream

cnasikas

Could you please add a unit test (x-pack/plugins/alerting/server/routes/rule/apis/find/find_rules_route.test.ts) to be sure we register the correct routes with the correct access?

x-pack/plugins/alerting/server/routes/rule/apis/find/find_rules_route.ts

dominiqueclarke

obs-ux-management changes LGTM

adcoelho

I approved the PR but I never liked how we structured the code in find_rules_route.ts and I think in part that is the reason we had the bug.

It is not the scope of this issue so I'll leave it up to you but I was thinking the following:

What is the point of having a buildFindRulesRoute function?

It is not exported anywhere else.
Its logic is completely different if the route is internal or external.
the if else being based on the URL doesn't feel right, what if I wanted to "build a find rules route" that is internal but with a different URL?
Why have the options as params? For the external API they are hardcoded, we can put them inside the if. For the internal route we don't need the options.

I think this aux buildFindRulesRoute function does more harm than good. We could move the code inside findRulesRoute and findInternalRulesRoute and have them be completely separate. (Maybe even separate files.)

cnasikas · 2024-09-30T09:20:40Z

+1 to what @adcoelho said. Same for the integration tests.

…nal-public-access

Zacqary · 2024-10-16T18:31:55Z

@elasticmachine merge upstream

cnasikas

Thanks for splitting the routes into different files! What do you think of splitting also the integration files into find.ts and find_internal.ts?

x-pack/test/alerting_api_integration/security_and_spaces/group1/tests/alerting/find.ts

cnasikas · 2024-10-21T13:47:18Z

x-pack/test/alerting_api_integration/spaces_only/tests/alerting/group1/find.ts

@@ -332,7 +332,6 @@ export default function createFindTests({ getService }: FtrProviderContext) {
    afterEach(() => objectRemover.removeAll());

    findTestUtils('public', supertest, objectRemover);
-    findTestUtils('internal', supertest, objectRemover);


Should we keep this?

….com/Zacqary/kibana into 192957-remove-internal-public-access

…nal-public-access

elasticmachine · 2024-10-21T23:18:40Z

💛 Build succeeded, but was flaky

Buildkite Build
Commit: 92d2a02
Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-193757-92d2a02d72e4

Failed CI Steps

Jest Tests #4

Test Failures

[job] [logs] Jest Tests #4 / AllCasesListGeneric Actions Row actions should delete a case

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`securitySolution`	20.7MB	20.7MB	+85.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`@kbn/test-suites-xpack`	730	732	+2

Total ESLint disabled count

id	before	after	diff
`@kbn/test-suites-xpack`	755	757	+2

History

💔 Build #244469 failed b64b9a9
💔 Build #244420 failed 1593634
💔 Build #244402 failed c350629
💚 Build #243251 succeeded 2934c11
💚 Build #242734 succeeded de002fa

cnasikas

Thank you for your patience with the review in this PR!

cnasikas · 2024-10-22T16:22:48Z

x-pack/test/alerting_api_integration/security_and_spaces/group1/tests/alerting/index.ts

@@ -22,6 +22,7 @@ export default function alertingTests({ loadTestFile, getService }: FtrProviderC

      loadTestFile(require.resolve('./backfill'));
      loadTestFile(require.resolve('./find'));
+      loadTestFile(require.resolve('./find_internal'));
      loadTestFile(require.resolve('./find_with_post'));


I think we do not need the find_with_post file anymore.

banderror

Rule Management changes LGTM. I pulled the PR and tested rule snoozing locally to confirm that our dependency on the internal find endpoint hasn't been impacted - everything seems to work fine.

Thanks @Zacqary 👍 ✅

kibanamachine · 2024-10-23T04:33:47Z

Starting backport for target branches: 8.16, 8.x

https://github.com/elastic/kibana/actions/runs/11473099213

…public access (elastic#193757) ## Summary Fixes elastic#192957 Removes the `internal/_find` route from public access by moving the hard-coded `options` into the route builder functions. --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com> (cherry picked from commit 196caba)

kibanamachine · 2024-10-23T04:38:23Z

💚 All backports created successfully

Status	Branch	Result
✅	8.16
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…I with public access (#193757) (#197358) # Backport This will backport the following commits from `main` to `8.16`: - [[ResponseOps][Rules] Remove unintended internal Find routes API with public access (#193757)](#193757)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Zacqary Adam Xeper <Zacqary@users.noreply.github.com>

… with public access (#193757) (#197359) # Backport This will backport the following commits from `main` to `8.x`: - [[ResponseOps][Rules] Remove unintended internal Find routes API with public access (#193757)](#193757)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)  Co-authored-by: Zacqary Adam Xeper <Zacqary@users.noreply.github.com>

[ResponseOps][Rules] Remove unintended internal Find routes API with …

8e116ea

…public access

Zacqary added release_note:fix Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v9.0.0 Feature:Alerting/RulesFramework Issues related to the Alerting Rules Framework v8.16.0 labels Sep 23, 2024

Zacqary requested a review from a team as a code owner September 23, 2024 16:47

Zacqary added the backport:version Backport to applied version labels label Sep 23, 2024

Merge branch 'main' into 192957-remove-internal-public-access

5bcccb3

cnasikas reviewed Sep 24, 2024

View reviewed changes

x-pack/plugins/alerting/server/routes/rule/apis/find/find_rules_route.ts Show resolved Hide resolved

cnasikas added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:version Backport to applied version labels labels Sep 24, 2024

cnasikas mentioned this pull request Sep 24, 2024

[RsponseOps][Alerting] Explicitly set access to all API routes of actions, connectors, rules, alerts, and cases plugins #193520

Merged

Zacqary added 2 commits September 24, 2024 13:15

Wrap get/post registrations in conditional

b1dd035

Add unit tests for proper access calls

ac11af5

Zacqary requested a review from cnasikas September 24, 2024 18:43

cnasikas approved these changes Sep 25, 2024

View reviewed changes

Change internal/find get to post in tests

1057825

Zacqary requested a review from a team as a code owner September 25, 2024 17:24

botelastic bot added the ci:project-deploy-observability Create an Observability project label Sep 25, 2024

dominiqueclarke approved these changes Sep 26, 2024

View reviewed changes

adcoelho approved these changes Sep 30, 2024

View reviewed changes

Zacqary added 4 commits October 1, 2024 11:10

Merge remote-tracking branch 'upstream/main' into 192957-remove-inter…

49ba566

…nal-public-access

Fix test post queries

c45f20c

Merge remote-tracking branch 'upstream/main' into 192957-remove-inter…

ece2d5c

…nal-public-access

Add kbn-xssrf to post tests

35f830e

Zacqary added 2 commits October 14, 2024 16:59

Change internal find intercept in cypress test

ddae6e8

Move int internal find route to own file

de002fa

Merge branch 'main' into 192957-remove-internal-public-access

2934c11

cnasikas requested review from cnasikas and adcoelho October 21, 2024 13:39

cnasikas reviewed Oct 21, 2024

View reviewed changes

Zacqary added 4 commits October 21, 2024 13:56

Split puSplit public and internal test suites

ef2f711

Merge branch '192957-remove-internal-public-access' of https://github…

1d9f67b

….com/Zacqary/kibana into 192957-remove-internal-public-access

Merge remote-tracking branch 'upstream/main' into 192957-remove-inter…

c350629

…nal-public-access

Fix internal test suite split

1593634

Zacqary enabled auto-merge (squash) October 21, 2024 18:48

Zacqary added 2 commits October 21, 2024 17:43

Fix incorrect public test urls

b64b9a9

Fix expected 200, change to 400

92d2a02

cnasikas approved these changes Oct 22, 2024

View reviewed changes

cnasikas disabled auto-merge October 22, 2024 16:24

cnasikas requested a review from maximpn October 22, 2024 16:25

banderror approved these changes Oct 22, 2024

View reviewed changes

Zacqary merged commit 196caba into elastic:main Oct 23, 2024
47 checks passed

kibanamachine mentioned this pull request Oct 23, 2024

[8.16] [ResponseOps][Rules] Remove unintended internal Find routes API with public access (#193757) #197358

Merged

kibanamachine mentioned this pull request Oct 23, 2024

[8.x] [ResponseOps][Rules] Remove unintended internal Find routes API with public access (#193757) #197359

Merged

kibanamachine added the v8.17.0 label Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ResponseOps][Rules] Remove unintended internal Find routes API with public access #193757

[ResponseOps][Rules] Remove unintended internal Find routes API with public access #193757

Zacqary commented Sep 23, 2024 •

edited by kibanamachine

Loading

elasticmachine commented Sep 23, 2024

obltmachine commented Sep 23, 2024

Zacqary commented Sep 23, 2024

cnasikas left a comment

dominiqueclarke left a comment

adcoelho left a comment

cnasikas commented Sep 30, 2024

Zacqary commented Oct 16, 2024

cnasikas left a comment •

edited

Loading

cnasikas Oct 21, 2024

elasticmachine commented Oct 21, 2024 •

edited

Loading

ESLint disabled line counts

Total ESLint disabled count

cnasikas left a comment

cnasikas Oct 22, 2024

banderror left a comment

kibanamachine commented Oct 23, 2024

kibanamachine commented Oct 23, 2024

[ResponseOps][Rules] Remove unintended internal Find routes API with public access #193757

[ResponseOps][Rules] Remove unintended internal Find routes API with public access #193757

Conversation

Zacqary commented Sep 23, 2024 • edited by kibanamachine Loading

Summary

elasticmachine commented Sep 23, 2024

obltmachine commented Sep 23, 2024

🤖 GitHub comments

Zacqary commented Sep 23, 2024

cnasikas left a comment

Choose a reason for hiding this comment

dominiqueclarke left a comment

Choose a reason for hiding this comment

adcoelho left a comment

Choose a reason for hiding this comment

cnasikas commented Sep 30, 2024

Zacqary commented Oct 16, 2024

cnasikas left a comment • edited Loading

Choose a reason for hiding this comment

cnasikas Oct 21, 2024

Choose a reason for hiding this comment

elasticmachine commented Oct 21, 2024 • edited Loading

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

Metrics [docs]

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

cnasikas left a comment

Choose a reason for hiding this comment

cnasikas Oct 22, 2024

Choose a reason for hiding this comment

banderror left a comment

Choose a reason for hiding this comment

kibanamachine commented Oct 23, 2024

kibanamachine commented Oct 23, 2024

💚 All backports created successfully

Questions ?

Zacqary commented Sep 23, 2024 •

edited by kibanamachine

Loading

cnasikas left a comment •

edited

Loading

elasticmachine commented Oct 21, 2024 •

edited

Loading